Skip to content

Add SyncMatchOutcome to hooks API with rate limiting signal#10045

Open
rkannan82 wants to merge 11 commits intokannan/sync-match-result-structfrom
kannan/add-rate-limited-to-task-hook-v2
Open

Add SyncMatchOutcome to hooks API with rate limiting signal#10045
rkannan82 wants to merge 11 commits intokannan/sync-match-result-structfrom
kannan/add-rate-limited-to-task-hook-v2

Conversation

@rkannan82
Copy link
Copy Markdown
Contributor

@rkannan82 rkannan82 commented Apr 23, 2026

What

Add SyncMatchOutcome enum to the hooks API (NotMatched, Success, RateLimited) and plumb rate limiting signal from the matcher through to hooks. Keep IsSyncMatch as deprecated for backwards compatibility.

Why

Hook consumers (e.g. scaling operators) need to distinguish rate limiting from genuine lack of pollers when deciding whether to scale up workers.

How did you test it?

Unit tests — rate-limited and non-rate-limited scenarios, multiple hooks invocation.

🤖 Generated with Claude Code

@rkannan82 rkannan82 force-pushed the kannan/add-rate-limited-to-task-hook-v2 branch from a3d7db8 to 6513520 Compare April 24, 2026 01:33
@rkannan82 rkannan82 requested a review from dnr April 24, 2026 01:41
@rkannan82 rkannan82 marked this pull request as ready for review April 24, 2026 01:41
@rkannan82 rkannan82 requested a review from a team as a code owner April 24, 2026 01:41
@rkannan82 rkannan82 force-pushed the kannan/add-rate-limited-to-task-hook-v2 branch 2 times, most recently from deffb5f to ae105f9 Compare April 24, 2026 01:47
@rkannan82 rkannan82 requested a review from ShahabT April 24, 2026 20:49
Comment thread service/matching/hooks/task_lifecycle_hooks.go Outdated
@rkannan82 rkannan82 force-pushed the kannan/add-rate-limited-to-task-hook-v2 branch from ae105f9 to 982f720 Compare April 28, 2026 21:58
@rkannan82 rkannan82 force-pushed the kannan/sync-match-result-struct branch 7 times, most recently from 5fdd8bd to 359d5e2 Compare April 28, 2026 23:06
@rkannan82 rkannan82 force-pushed the kannan/add-rate-limited-to-task-hook-v2 branch 3 times, most recently from 5334077 to 4d0329d Compare April 29, 2026 01:38
@rkannan82 rkannan82 changed the title Add IsRateLimited to TaskAddHookDetails Add NoMatchReasonRateLimited and plumb to hooks Apr 29, 2026
@rkannan82 rkannan82 changed the title Add NoMatchReasonRateLimited and plumb to hooks Propagate rate limited reason through hooks interface Apr 29, 2026
@rkannan82 rkannan82 requested a review from ShahabT April 29, 2026 01:46
Add NoMatchReasonRateLimited to the NoMatchReason enum. When
findAndWakeMatches detects rate limiting, MatchTaskImmediately returns
this reason, which flows through Offer → TrySyncMatch → processTaskAddHooks.

At the hooks boundary, internal NoMatchReason is translated to
hooks.NoMatchReason (which only exposes Unspecified and RateLimited).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@rkannan82 rkannan82 force-pushed the kannan/add-rate-limited-to-task-hook-v2 branch from 4d0329d to 7e66f5f Compare April 29, 2026 01:47
var result syncMatchResult
result, err = syncMatchQueue.TrySyncMatch(ctx, syncMatchTask)
syncMatched = result.matched
// TODO: TrySyncMatch returns matched=true with a start error (e.g. busy workflow),
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ShahabT @dnr See if this makes sense.

rkannan82 and others added 3 commits April 29, 2026 11:07
Resolve merge conflicts after parent PR switched from syncMatchResult
struct to syncMatchOutcome enum. Add syncMatchRateLimited to the enum
and update all references.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace the IsSyncMatch bool and NoMatchReason enum with a single
SyncMatchOutcome enum in the hooks API. The internal syncMatchOutcome
is translated to the hooks-specific enum at the boundary.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
rkannan82 and others added 3 commits April 29, 2026 11:20
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Retain the IsSyncMatch bool field in TaskAddHookDetails, derived from
SyncMatchOutcome, so existing hook consumers are not broken.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Ensures uninitialized SyncMatchOutcome fields are distinguishable
from intentional NotMatched values.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@rkannan82 rkannan82 changed the title Propagate rate limited reason through hooks interface Add SyncMatchOutcome to hooks API with rate limiting signal Apr 29, 2026
rkannan82 and others added 4 commits April 29, 2026 11:26
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Map syncMatchUnspecified to hooks.SyncMatchOutcomeUnspecified so
the bug signal propagates rather than being silently mapped to
NotMatched.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants